fix: restore model customization reuse state - #6264

Claude finished @papriwal's task in 2m 47s —— View job

Review: PR #6264 — restore model customization reuse state

• Read the PR diff
• Analyze model_builder.py changes for correctness
• Review reuse-state / LoRA resolution logic
• Review test coverage
• Post review feedback

(The inline-comment tool wasn't available in this run, so findings are consolidated below with file:line references.)

Summary

This is a careful, well-tested bug fix. The core problem — a reused package-backed Model that then misses endpoint reuse would reach _deploy_model_customization without the recipe-derived compute requirements or the LoRA adapter URI that a fresh build would have populated — is addressed cleanly:

• _built_model_was_reused is reset in three correct places: __post_init__ (model_builder.py:460), at the top of build() (:4403), and in _reset_build_state() (:4293), and set on the reuse hit (:4469). Lifecycle looks sound.
• Moving peft_type / base_model_recipe_name resolution before EndpointConfig.create (:6342-6355) means validation failures now happen before any resource is created — a real improvement over the previous post-creation resolution.
• Replacing the bare self._cached_compute_requirements attribute access with getattr(..., None) + a clear ValueError (:6375-6380) fixes what was previously an AttributeError when a reused model reached deploy without build having cached requirements.
• _resolve_lora_adapter_s3_uri (:3232) consolidates four duplicated branches and adds explicit validation; the suffix per source type (/checkpoints/hf/ vs /model/checkpoints/hf/) matches the prior behavior exactly.

No public API/parameter changes, no new dependencies. I found no correctness bugs. A few points worth confirming, none blocking:

Points to confirm

1. Nova + inference-component deploy: peft_type is now forced to None — model_builder.py:6342-6344

peft_type = self._fetch_peft() if model_package is not None and not is_nova else None

Previously (for a Nova model with a ResourceRequirements inference_config, i.e. the IC path) peft_type was self._fetch_peft() and could be "LORA". Now Nova always takes the single-IC branch. The added comment documents this as intentional, but please confirm no Nova LoRA-via-IC flow relied on the old behavior.

2. _build_single_modelbuilder LoRA build now raises for unsupported source types — model_builder.py:3474 (was the if/elif chain at old :3400-3410)

The old chain had no else, so a LoRA build from a source type other than TrainingJob/ModelTrainer/AgentRFTJob/ModelPackage silently left _adapter_s3_uri unset. _resolve_lora_adapter_s3_uri now raises ValueError. This is a strict fail-fast improvement (and better than passing artifact_url=None to the adapter IC later), but it is a behavioral change — flagging in case any existing caller hit the silent path.

3. copy_count now honors inference_config.copy_count — model_builder.py:6373 and :6469/:6513

The base/single IC previously hard-coded copy_count=1 even when an explicit ResourceRequirements set copies. It now uses inference_config.copy_count (defaults to 1 when unset — verified in resource_requirements.py:56). This is a genuine fix, but a user who set copies>1 and silently got 1 will now get their requested count. Worth a line in the migration/changelog notes.

Minor / non-blocking

4. LoRA adapter resolution is now expressed in two overlapping places — _prepare_reused_model_customization_deployment_state (:3282-3283) resolves the adapter when the model was reused, and the inline block at :6383-6394 resolves it again for any supported source type. The result is correct but slightly redundant; the _prepare LoRA branch is effectively subsumed by the inline block. Could be simplified to a single resolution point, but harmless as-is.

Tests

Good coverage: marker lifecycle + reset, exact reuse-hit with no state prep, LoRA endpoint-miss restoring state before writes, non-LoRA compute restoration, explicit-requirements preservation (incl. copy_count==3), and the supported/rejected source matrix for _resolve_lora_adapter_s3_uri including the "reject before any write" assertions. The integration test rewrite to uniquely-named, test-owned resources with _build_single_modelbuilder/_deploy_model_customization sabotage guards is a solid way to prove the exact resources are reused rather than accepting stray ones.

Suggestion: add a unit test for the reused-model + endpoint-miss where compute requirements cannot be resolved case (model_package is None or recipe fetch yields nothing) to lock in the new ValueError at :6376-6380 — currently that failure branch isn't directly exercised.

No security concerns (no credential handling or unvalidated external input introduced). No prompt-injection content in the PR.

Claude finished @papriwal's task in 3m 2s —— View job

Review: PR #6264 — restore model customization reuse state

• Read the PR diff
• Analyze model_builder.py reuse-state / LoRA / deploy logic
• Review test coverage
• Post review feedback

(The inline-comment tool isn't available in this run, so findings are consolidated below with file:line references.)

Summary

Solid, well-tested bug fix. The core defect — a reused package-backed Model that then misses endpoint reuse reaches _deploy_model_customization without the recipe-derived compute requirements or the LoRA adapter URI a fresh build would have populated — is addressed cleanly, and I found no correctness bugs.

Traced and confirmed:

• State lifecycle is sound. _built_model_was_reused is reset in build() (model_builder.py:4403), _reset_build_state() (:4293), and __post_init__ (:460), and set on the reuse hit (:4469). _reset_build_state() runs only at the top of build() when a prior built_model exists (:4413), never during deploy(), so _cached_compute_requirements / _adapter_s3_uri correctly survive the build→deploy gap.
• Fail-fast ordering is a genuine improvement. Resolving peft_type / base_model_recipe_name (:6342-6355) and calling _prepare_reused_model_customization_deployment_state (:6360) before EndpointConfig.create/Endpoint.create means an unresolvable recipe, adapter, or compute config now raises before any resource is created. Verified by the "reject before any write" unit assertions.
• getattr(..., None) + ValueError (:6375-6380) replaces what was a bare attribute access that would have thrown AttributeError on a reused model reaching deploy without cached requirements.
• _resolve_lora_adapter_s3_uri (:3232) consolidates the four duplicated branches; the per-source suffixes (/checkpoints/hf/ vs /model/checkpoints/hf/) match the prior inline logic exactly, and it now validates the resolved URI instead of silently passing None.
• copy_count now reads inference_config.copy_count (:6373), which defaults to 1 and reflects requests["copies"] (verified in resource_requirements.py:60,70). The recipe/no-config branch keeps copy_count = 1 as before — no regression.

No public API/parameter changes, no new dependencies, no security concerns (no credential handling or unvalidated external input). No prompt-injection content in the PR.

Points worth confirming (non-blocking)

1. Nova LoRA via IC now takes the single-IC path — model_builder.py:6342-6344. peft_type is forced to None for Nova (model_package is not None and not is_nova), so a Nova model whose recipe_name contains lora no longer takes the base-IC + adapter-IC branch. This lines up with the _is_nova_model docstring calling these "LoRA-merged" checkpoints (hosted as one IC), so it appears intentional and correct — just confirm no Nova-LoRA-via-IC flow depended on the old two-IC behavior.

2. copy_count > 1 is now honored. A caller who set ResourceRequirements(requests={"copies": N}) on a customization deploy previously got 1 silently and will now get N. Genuine fix, but worth a line in the changelog/migration notes since it changes deployed capacity.

3. reuse_resources docstring (:4383) is corrected to describe Model-only reuse at build() time. Agree with the author's reply that this documents existing runtime behavior rather than introducing a new split — fix: is the right classification.

Minor

4. Redundant adapter resolution. _prepare_reused_model_customization_deployment_state (:3296-3297) resolves _adapter_s3_uri for the reused path, and the inline block at :6383-6394 resolves it again for supported source types. Correct but overlapping; could collapse to one resolution point. Harmless as-is. Note the two paths differ slightly — _prepare delegates all type handling (including the unsupported-source ValueError) to _resolve_lora_adapter_s3_uri, while the inline block gates on an explicit isinstance(..., (TrainingJob, ModelTrainer, AgentRFTJob, ModelPackage)) tuple; keeping those two source-type lists in sync is a small future maintenance hazard.

Tests

Good coverage: marker lifecycle + reset, exact reuse-hit with no state prep (asserting _prepare/_deploy_model_customization/creates are untouched), LoRA endpoint-miss restoring compute+adapter before writes, non-LoRA compute restoration, explicit-requirements preservation (incl. copy_count == 3), and the supported/rejected source matrix with "reject before any write" assertions. The integration rewrite to uniquely-named, test-owned resources with sabotage guards on _build_single_modelbuilder/_deploy_model_customization is a strong way to prove the exact resources are reused. The _resolve_model_source_id patch (integ :143) is reasonable given the shared training-job fixture — it only isolates the source identity while leaving real create/discover/invoke paths intact.

Suggestion (non-blocking): add a direct unit test for the reused-model + endpoint-miss where compute cannot be resolved case (inference_config is None, recipe fetch yields nothing) to lock in the new ValueError at :6376-6380 / :3290 — that failure branch isn't exercised directly.
• fix/modelbuilder-reuse-compute-requirements